Prepare bounded command transport and guarded ACP writes by jhgaylor · Pull Request #1748 · managoat/fountain

jhgaylor · 2026-09-08T02:22:41Z

A bounded turn needs its provider command tracked even when the conversation actor blocks or the turn succeeds. This adds a supervised transport that binds trusted command identity, checks durable authority before each ACP write, and retains cleanup obligations for every outcome. Unknown spawns and uncertain writes are never replayed. Successful turns require confirmed cleanup before a fresh connection can proceed.

Stacked on #1747. Draft; public enforcement remains disabled. The conversation actor does not select this transport yet. Activation still requires tracked adapter setup, complete actor/cancellation/recovery wiring, released Sprites identity support, and live acceptance. Local shutdown and a completed reply do not prove remote termination or stopped billing.

Validation: full precommit 4,699 tests and 6 doctests, zero failures; 66 focused tests; 20 independent PostgreSQL races. A real ACP peer runs through the guarded writer against a simulated provider, then refuses a post-completion prompt. Tests cover identity forgery/conflicts, delayed spawn results, blocked writes, owner death, durable close acknowledgment, orphan task timeouts, terminal outcome arbitration and redacted crash state. No provider operations occurred. ADR 0046 and decisions/evidence/execution-transport.json record scope and evidence.

Tracks #1732. Internal preparation only; sdk-no-release applies.

All 24 CI checks pass or intentionally skip at 9b1fb2ea8a26e9d32823d7c0e5c0a8ed5f2b4a83.

Part of #1732 — held. Superseded on main by #1773–#1793; the residual hunks are being re-cut as focused PRs tracked by #1864, which keeps these frozen until each has a replacement or an explicitly linked deferral. Do not close: they are the reference for that mapping.

jhgaylor

Posted under the wrong account by mistake (this repo has two authenticated identities and the CLI reverted mid-run). The review itself is on this PR from @BinaryBourbon — read that one; this placeholder carries no findings.

BinaryBourbon

Review: spawn-intent-before-I/O is right; the provider story needs stating

Commenting. The transport is the piece that makes the journal mean something, and the ordering is correct: _unsafe_claim_spawn/2 persists intent before DynamicSupervisor.start_child, stdin stays closed until identity binds, each write rechecks the journal, and a failed write retires the whole execution instead of retrying it. restart: :temporary is the right supervision choice for something that must never be replayed. Buffer caps (@buffer_bytes, @buffer_frames) and the :deadline / :drain_end pair are good defensive detail.

Points:

It is Sprites-only, and that is not visible from the outside. _unsafe_start/5 matches %TurnExecution{provider: "sprites"} and otherwise returns :provider_not_supported. Combined with #1749's if sandbox.provider != "sprites", do: Repo.rollback(:provider_not_supported) and #1746's four-entry @providers map, the stack reads as multi-provider but is not. E2B, Daytona and self-hosted runners are a real part of this product (ADR 0018, ADR 0022) — the moduledoc and ADR 0046 should say plainly that bounded turns are Sprites-only for now and what the others get instead (today: silently no bound, which is the outcome that worries me most, because a caller that asks for a limit and is refused is fine, and one that asks and is ignored is not).
remaining is computed once in init/1 from execution.deadline_at. That is correct while the process lives, and the journal plus #1746's coordinator cover the restart. Worth one line saying so, because the timer looks like the enforcement mechanism at a glance and it is not.
call/3 rescues :exit into {:error, :transport_unavailable}. Fine, but that swallows a genuine timeout as well as a dead process, and the two mean different things to the journal. If a 30 s GenServer.call times out while the transport is alive and mid-write, the caller sees :transport_unavailable and the execution may still be writing. Distinguishing them would make the failure mode readable.

Blocked behind #1744/#1745/#1746 and needs the same rebase.

…rate a timeout from a dead transport Review notes from #1748, plus one cross-PR semantic conflict the rebase surfaced. **Sprites-only is now stated rather than implied.** `_unsafe_start/5` refuses every other provider and admission rolls back to match, because binding a provider-issued session id from trusted control metadata is a per-adapter capability and only the Sprites adapter has it. The moduledoc and ADR 0046 say so, and a regression asserts that e2b, daytona and runner are refused *without* recording spawn intent — so the turn stays retryable instead of landing in the `awaiting_identity` fence. The important half is that those providers get a refusal at admission and not a silently unbounded turn. **A timeout and a dead transport are different answers.** `call/3` collapsed both into `:transport_unavailable`, which reads as "nothing happened" and is only true of the second: a transport that did not answer in 30 s may be mid-write. Timeouts now return `:transport_timeout`. `write/2` gained an optional timeout so the regression costs 50 ms rather than 30 s. **The `:deadline` timer in `init/1` is documented as a convenience, not the mechanism.** The guarantee is the absolute `deadline_at` on the journal row, which the coordinator acts on whether or not this process survived. **The semantic conflict.** This PR changes completion: every bounded connection now owes remote cleanup, a successful reply included, so a completed turn's row lands in `ready` rather than `completed` and `prior_connection/1` refuses all reuse. That is right — a runtime that answered can still hold background work, which is the shape behind the phantom follow-up turns — but it also means the fence now reaches the happy path, and a #1744 regression asserting the old shape failed here. Updated to drive the row to a confirmed stop, and a second regression covers the corollary: a turn that answered correctly whose cleanup is then lost ages out, leaving its own outcome untouched. ADR 0046 records the change and why the ageing exit is load-bearing rather than a corner case. Full core suite: 6 doctests, 5,022 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9reKpmXUUJf4eUMULogie Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>

jhgaylor · 2026-09-11T06:34:36Z

Force-pushed 9e429567. Rebased onto the fixed #1747, all three notes addressed, plus one thing the rebase surfaced that I think matters more than any of them.

Sprites-only is now stated rather than implied. The moduledoc and ADR 0046 both say it, with the reason: binding a provider-issued session id from trusted control metadata is a per-adapter capability and only the Sprites adapter has it, so without one there is nothing to terminate by name. A regression asserts e2b, daytona and runner are refused without recording spawn intent — the turn stays retryable instead of landing in the awaiting_identity fence. The half I cared about holds: those providers get a refusal at admission, not a silently unbounded turn.

A timeout and a dead transport are different answers. call/3 collapsed both into :transport_unavailable, which reads as "nothing happened" and is only true of the second — a transport that did not answer in 30 s may be mid-write. Timeouts now return :transport_timeout. write/2 gained an optional timeout so the regression costs 50 ms instead of 30 s.

The init/1 timer is documented as a local convenience rather than the mechanism; the guarantee is deadline_at on the row, which the coordinator acts on whether or not the process survived.

The thing worth your attention: this PR changes completion semantics, and I only noticed because a #1744 regression failed on it.

complete/3 now routes a successful turn through stop/4, so a completed bounded turn lands in ready rather than completed, and prior_connection/1 refuses all connection reuse. The comment gives the reason and I agree with it — a runtime that answered can still hold background work, which is exactly the shape behind the phantom follow-up turns.

But the consequence is that the termination fence now reaches the happy path. Every successful bounded turn owes a provider cleanup, so a lost or failed terminate_session on a turn that worked perfectly fences its conversation and its shared home — the same dead-end I flagged on #1744, now on the common path rather than the error path. The ageing exit added there is load-bearing because of this PR, not despite it. I have added a regression for that corollary (a successful turn whose cleanup is lost ages out, with its own completed outcome left untouched) and ADR 0046 now records both the change and why.

That is worth a second look from you, because it is the sort of change that reads as a small refactor in the diff — three call sites moved to stop/4 — and is actually a shift in what the journal promises.

Verification. Two new regressions here plus one updated from #1744. Reverting the provider guard and the timeout clause fails three tests. Full core suite: 6 doctests, 5,022 tests, 0 failures, 2 skipped. Format, --warnings-as-errors, credo --strict (7,355 mods/funs, no issues), okf validate decisions clean.

On to #1749 — the one I requested changes on.

…rate a timeout from a dead transport Review notes from #1748, plus one cross-PR semantic conflict the rebase surfaced. **Sprites-only is now stated rather than implied.** `_unsafe_start/5` refuses every other provider and admission rolls back to match, because binding a provider-issued session id from trusted control metadata is a per-adapter capability and only the Sprites adapter has it. The moduledoc and ADR 0046 say so, and a regression asserts that e2b, daytona and runner are refused *without* recording spawn intent — so the turn stays retryable instead of landing in the `awaiting_identity` fence. The important half is that those providers get a refusal at admission and not a silently unbounded turn. **A timeout and a dead transport are different answers.** `call/3` collapsed both into `:transport_unavailable`, which reads as "nothing happened" and is only true of the second: a transport that did not answer in 30 s may be mid-write. Timeouts now return `:transport_timeout`. `write/2` gained an optional timeout so the regression costs 50 ms rather than 30 s. **The `:deadline` timer in `init/1` is documented as a convenience, not the mechanism.** The guarantee is the absolute `deadline_at` on the journal row, which the coordinator acts on whether or not this process survived. **The semantic conflict.** This PR changes completion: every bounded connection now owes remote cleanup, a successful reply included, so a completed turn's row lands in `ready` rather than `completed` and `prior_connection/1` refuses all reuse. That is right — a runtime that answered can still hold background work, which is the shape behind the phantom follow-up turns — but it also means the fence now reaches the happy path, and a #1744 regression asserting the old shape failed here. Updated to drive the row to a confirmed stop, and a second regression covers the corollary: a turn that answered correctly whose cleanup is then lost ages out, leaving its own outcome untouched. ADR 0046 records the change and why the ageing exit is load-bearing rather than a corner case. Full core suite: 6 doctests, 5,022 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9reKpmXUUJf4eUMULogie Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>

BinaryBourbon

Review: the provider story is stated, and stated in the right place

Re-reviewed at 6d51262. All three notes answered.

Sprites-only. The moduledoc and ADR 0046 now say it plainly, and the regression is better than the sentence: for e2b, daytona and runner it asserts spawn_submitted_at == nil and state == "active" after the refusal. That is the half I actually cared about — a refused provider leaves a retryable turn rather than an awaiting_identity row somebody has to age out. "A caller that asks for a limit and is told no is fine; one that asks and is ignored is not" reads back as the rule it is.

Timeout versus dead transport. :exit, {:timeout, _} ahead of :exit, _ is the right order, and the test distinguishes them with a live-but-silent process and a genuinely dead one instead of asserting the clause exists.

One thing worth a sentence somewhere, because it will otherwise be assumed: nothing acts on the difference yet. At the stack tip, :transport_timeout appears only in execution_transport.ex and its test — TurnMachine.command_writer/2 hands both to the ACP writer identically, and Connection._unsafe_spawn_bounded/5 closes on {:error, _} either way. Today this buys a truthful return value and a readable log line, which is enough; a line in the moduledoc saying "no caller branches on this yet" stops the next reader concluding the journal already treats them differently.

The :deadline timer as convenience rather than mechanism is now said out loud, which was the point.

The semantic conflict is the interesting part of this re-cut. Every bounded connection owing cleanup — a successful reply included — so a completed turn lands in ready and prior_connection/1 refuses all reuse is right, and it is right for the reason given (a runtime that answered can still hold background work; that is the phantom-follow-up shape). Updating the #1744 regression instead of weakening the state machine is the correct direction, and the companion test — a turn that answered correctly whose cleanup is then lost ages out with its own completed outcome untouched — is the one that makes the ageing exit load-bearing rather than a corner case. ADR 0046 records it.

Approving.

…rate a timeout from a dead transport Review notes from #1748, plus one cross-PR semantic conflict the rebase surfaced. **Sprites-only is now stated rather than implied.** `_unsafe_start/5` refuses every other provider and admission rolls back to match, because binding a provider-issued session id from trusted control metadata is a per-adapter capability and only the Sprites adapter has it. The moduledoc and ADR 0046 say so, and a regression asserts that e2b, daytona and runner are refused *without* recording spawn intent — so the turn stays retryable instead of landing in the `awaiting_identity` fence. The important half is that those providers get a refusal at admission and not a silently unbounded turn. **A timeout and a dead transport are different answers.** `call/3` collapsed both into `:transport_unavailable`, which reads as "nothing happened" and is only true of the second: a transport that did not answer in 30 s may be mid-write. Timeouts now return `:transport_timeout`. `write/2` gained an optional timeout so the regression costs 50 ms rather than 30 s. **The `:deadline` timer in `init/1` is documented as a convenience, not the mechanism.** The guarantee is the absolute `deadline_at` on the journal row, which the coordinator acts on whether or not this process survived. **The semantic conflict.** This PR changes completion: every bounded connection now owes remote cleanup, a successful reply included, so a completed turn's row lands in `ready` rather than `completed` and `prior_connection/1` refuses all reuse. That is right — a runtime that answered can still hold background work, which is the shape behind the phantom follow-up turns — but it also means the fence now reaches the happy path, and a #1744 regression asserting the old shape failed here. Updated to drive the row to a confirmed stop, and a second regression covers the corollary: a turn that answered correctly whose cleanup is then lost ages out, leaving its own outcome untouched. ADR 0046 records the change and why the ageing exit is load-bearing rather than a corner case. Full core suite: 6 doctests, 5,022 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9reKpmXUUJf4eUMULogie Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>

…transaction The review of 1382ad5 is right and the reasoning generalizes: write authorization belongs where a write happens. `handle_info/2` called `_unsafe_authorize_write/3` for every message an actor received while a bounded turn was open — every `{:stdout, ...}` chunk, every `{:acp, ...}` report, `:lifecycle_check`, every `:DOWN`. That function is a transaction with `FOR UPDATE` on the conversation, the journal row and the turn: six queries and three row locks, hundreds to thousands of times per chatty turn. Worse than the cost, holding the parent lock serialized the actor against admission, release, reset and the coordinator's own expire, so the hotter the turn the longer the coordinator queued behind the very turn it was supposed to expire. The inbound stream cannot reach the provider by itself, so it never needed write authorization. `ExecutionGuard._unsafe_actor_gate/3` reads three columns — state, connection, deadline — unlocked, and answers only "is this still mine, still inside its deadline". Its `:retire` is not the durable decision: retirement takes the locks one frame later, where `_unsafe_complete/3` arbitrates completion against expiry. The authoritative checks stay at the transport (#1748) and there. The regression measures the property rather than asserting it: one query, matching `^SELECT`, with no `FOR UPDATE`, no `FOR SHARE` and no `BEGIN` — collected from telemetry and asserted in the test body, because a raising handler is detached and fails nothing (#1427). **`retire_bounded_turn/1` no longer takes the actor down with it.** It hard-matched `{:ok, decision}`, and `:not_found` is reachable: the journal deliberately carries no foreign key to its parent, so a row can be gone while an actor is still draining its mailbox. It now fails the local turn — the journal was the thing that would have ended it — clears `turn_execution` and closes. The first version of that test passed against the unfixed code, which is the only reason the missing `current_turn` clear was found; it now reproduces the `MatchError`. Both halves moved into `Fountain.Conversations.BoundedTurn`: journal logic the actor calls rather than actor logic, with `finish_acp_turn/4` passed in because ending a turn writes through the actor's transcript. That is also where the 45 lines came from — the pin ratchets 2739 -> 2735 rather than being raised. Three notes answered in comments: why `bounded_sdk_limits/2`'s hard match is safe (admission already called the same function through a `with`), what makes `_unsafe_interrupt/1`'s `limit: 1` exact (`turn_executions_open_conversation_index` is unique and partial), and why a bounded turn generates no title — titling is a second inference call the journal neither bounds nor prices, so a conversation whose first turn is bounded has no title until an unbounded one follows. That last one is a real gap and ADR 0046 now records it rather than leaving it silent. Full core suite: 6 doctests, 5,042 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9reKpmXUUJf4eUMULogie Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>